Revert "Add column add/drop/rename events"#15984
Conversation
This reverts commit 33fdaa0. The current implementation has the following problems: - does not work for Hive connector with `hive.security=system` (#15225 (comment)) - notifications are propagated even if transaction doesn't end up committing the DDL changes (#15225 (comment)) - notification propagation may disrupt transaction finish even if changes already applied in the external system (#12535 (comment)), for any connector that's using SYSTEM security (Iceberg, Delta with system security, or any JDBC connector)
|
Of course, we can also fix the functionality, instead of revert, if a fix exists. cc @trinodb/maintainers |
|
CI failed with an Ubuntu update problem, going to be fixed by #15840 |
findepi
force-pushed
the
revert-15225-david.stryker/add-column-change-callbacks
branch
from
d404494 to
15f573e
Compare
|
(updated commit message, no other changes) |
|
CI #15992 |
djsagain
left a comment
djsagain
left a comment
There was a problem hiding this comment.
Sorry that this caused trouble.
No worries @djsstarburst and thank you for your review! |
electrum
left a comment
electrum
left a comment
There was a problem hiding this comment.
It seems like we can fix this trivially by doing the getTableMetadata() before the column operation. It should be guarded by the security management check, so that we don't fetch it unnecessarily.
So add column would look like
@Override
public void addColumn(Session session, TableHandle tableHandle, ColumnMetadata column)
{
CatalogHandle catalogHandle = tableHandle.getCatalogHandle();
CatalogMetadata catalogMetadata = getCatalogMetadataForWrite(session, catalogHandle);
ConnectorMetadata metadata = catalogMetadata.getMetadata(session);
SchemaTableName tableName = null;
if (catalogMetadata.getSecurityManagement() != CONNECTOR) {
tableName = getTableMetadata(session, tableHandle).getTable();
}
metadata.addColumn(session.toConnectorSession(catalogHandle), tableHandle.getConnectorHandle(), column);
if (catalogMetadata.getSecurityManagement() != CONNECTOR) {
CatalogSchemaTableName sourceTableName = new CatalogSchemaTableName(catalogHandle.getCatalogName(), tableName);
systemSecurityMetadata.columnCreated(session, sourceTableName, column.getName());
}
}
That solves some, but not all, problems outlined above. Anyway, looking forward in seeing this in a PR. |
|
Marked this as a Talking with @electrum @djsstarburst offline about this PR. |
electrum
left a comment
electrum
left a comment
There was a problem hiding this comment.
I didn’t realize this is correctness issue in an existing connector, when not using this feature. We should revert.
|
Thank you @electrum for your review! |
|
CI #14637 |
findepi
deleted the
revert-15225-david.stryker/add-column-change-callbacks
branch
3 participants
Reverts #15225
The current implementation has the following problems
hive.security=system(Add column add/drop/rename events #15225 (comment))The revert is only temporary, the functionality will be reintroduced soon.
Fixes #12535